feat: add direction prop to PaperProvider and useLocale hook for RTL support#4921
feat: add direction prop to PaperProvider and useLocale hook for RTL support#4921hristototov wants to merge 4 commits intov6from
Conversation
…into @hristototov/feat/improve_RTL_handling
|
The mobile version of example app from this branch is ready! You can see it here. |
| <PortalHost> | ||
| <SettingsProvider value={settingsValue}> | ||
| <ThemeProvider theme={theme}>{children}</ThemeProvider> | ||
| <LocaleProvider value={{ direction }}> |
There was a problem hiding this comment.
We get a new object on every render and this affects FPS and re-render counts on screens that use Paper components.
We already memoizes theme and settingsValue. Do the same for this too.
|
Hey @hristototov, thank you for your pull request 🤗. The documentation from this branch can be viewed here. |
satya164
left a comment
satya164
left a comment
There was a problem hiding this comment.
Thanks for the PR. I left some comments. In addition, lets do the following:
- Export a
LocaleProviderlike done forThemeProvider, so user can wrap a specific subtree in a different locale if needed. The defaults etc., can live in this component directly instead of agetDefaultDirectionexport - Inherit the nearest locale in
Portal.tsxas done for the theme - Update documentation. Here is an example https://reactnavigation.org/docs/navigation-container/#direction
| export const LocaleContext = React.createContext<LocaleContextValue>({ | ||
| direction: getDefaultDirection(), | ||
| }); | ||
|
|
||
| export const { Provider: LocaleProvider } = LocaleContext; | ||
|
|
||
| export const useLocale = () => React.useContext(LocaleContext); |
There was a problem hiding this comment.
It shouldn't provide a default locale in the context itself. It must be provided when rendering the provider so it stays consistent, and components outside the tree properly throw.
The useLocale hook should throw an error when used outside the provider.
| export const LocaleContext = React.createContext<LocaleContextValue>({ | |
| direction: getDefaultDirection(), | |
| }); | |
| export const { Provider: LocaleProvider } = LocaleContext; | |
| export const useLocale = () => React.useContext(LocaleContext); | |
| export const LocaleContext = React.createContext<LocaleContextValue | undefined>(); | |
| export const { Provider: LocaleProvider } = LocaleContext; | |
| export function useLocale() { | |
| const locale = React.useContext(LocaleContext); | |
| if (locale === undefined) { | |
| throw new Error("Couldn't find a locale. Is your component inside PaperProvider?"); | |
| } | |
| return locale; | |
| } |
There was a problem hiding this comment.
I've addressed the three bullet points above.
However, the original task in Notion asked for a default value: https://www.notion.so/callstack/Improve-RTL-handling-in-paper-32e5d027c0f8807caf67e6cdbc6d4d74?v=3285d027c0f880a68ccb000c1ce3c56e
In your comment, you suggest we don't have one in order to be consistent. However, theme does have a default value MD3LightTheme, which means components using useTheme work without any wrapper in tests.
If we were to not have a default value for locale, then we'd have to update all (~46) test suites to wrap renders with a LocaleProvider (since useLocale would throw outside one), which I think is a bit too much for this PR. LMK your thoughts and if I got something wrong.
There was a problem hiding this comment.
the original task in Notion asked for a default value
Default value in the provider, not in the context itself.
then we'd have to update all (~46) test suites to wrap renders with a LocaleProvider
That's fine. Tests should be testing a real scenario - where users will have PaperProvider - so tests should use that.
5 participants
Motivation
Components in react-native-paper read RTL state directly from
I18nManager.getConstants().isRTL. This has two issues:I18nManageris a no-op on React Native Web, so RTL breaks entirely on WebSolution
New:
useLocale()hookDefaults to I18nManager.getConstants().isRTL when not provided, so existing apps are unaffected. Setting the prop does not call I18nManager.forceRTL or modify any native state; it only tells Paper which direction to use for rendering.
Related issue
N/A
Notion link
Test plan
Remove the prop and verify the app falls back to I18nManager (default behaviour unchanged)